Miscellaneous improvements related to durable task hub #4641
Conversation
…ions into mwf/durable-feedback
MicroFish91
changed the title
MicroFish91
changed the title
MicroFish91
changed the title
MicroFish91
changed the title
MicroFish91
force-pushed
the
mwf/durable-feedback
branch
from
1af8e52 to
001ac6b
Compare
|
Different model? |
|
Yeah, GPT-5 but also running off a prompt.markdown with some more specific instructions. I'm tweaking some of the priorities for reviewing. |
|
|
||
| export class DTSStartingResourcesLogStep<T extends IDTSAzureConnectionWizardContext> extends AzureWizardPromptStep<T> { | ||
| public hideStepCount: boolean = true; | ||
| protected hasLogged: boolean = false; | ||
|
|
||
| public async configureBeforePrompt(context: T): Promise<void> { |
There was a problem hiding this comment.
Potential duplicates after removing guard. Without hasLogged, configureBeforePrompt can prepend items multiple times if the step re-runs (back/forward). Does prependOrInsertAfterLastInfoChild dedupe using stepId? If not, please re-add a guard or enforce deduplication.
Review generated with Copilot
There was a problem hiding this comment.
No, but prompt steps will automatically remove activity children by step id when you go back.
|
|
||
| promptSteps.push(new FunctionAppUserAssignedIdentitiesListStep(dtsContributorRole /** targetRole */, { identityAssignStepPriority: 180 })); | ||
| executeSteps.push(new RoleAssignmentExecuteStep(this.getDTSRoleAssignmentCallback(context, dtsContributorRole), { priority: 190 })); | ||
|
|
There was a problem hiding this comment.
Bug: Pre-check may skip when creating a new hub. Here scopeId: context.dtsHub?.id (line 78) is undefined during hub creation, so FunctionAppUserAssignedIdentitiesListStep.configureBeforePrompt short-circuits and doesn’t verify an existing identity with the target role. Consider running the pre-check against the scheduler scope first (context.dts.id), or move the pre-check after DurableTaskHubCreateStep so scopeId is available.
Review generated with Copilot
There was a problem hiding this comment.
This scenario is fine and not a bug. If it's undefined at this time, that means the dtsHub does not currently exist and will be newly created. It will be assigned the role to whichever user assigned identity is chosen. If it doesn't exist yet, there's no reason to check for an existing match so the pre-check is good to skip via the guard clause.
| if (context.managedIdentity) { | ||
| context.newDTSConnectionSettingValue = context.newDTSConnectionSettingValue.replace(clientIdKey, context.managedIdentity?.clientId ?? clientIdKey); | ||
| } | ||
|
|
||
| context.valuesToMask.push(context.newDTSConnectionSettingValue); | ||
| } | ||
|
|
There was a problem hiding this comment.
ClientId placeholder may persist. If context.managedIdentity?.clientId is undefined, the replacement keeps the placeholder clientIdKey in the connection string (lines 16-23). Should this step validate a real clientId and fail fast, or is it guaranteed upstream by shouldExecute/flow?
Review generated with Copilot
There was a problem hiding this comment.
Not super worried because the if statement already verifies the managed identity exists. I could throw a nonNull error here if you'd prefer that.
| @@ -22,7 +24,7 @@ export class DurableTaskHubListStep<T extends IDTSAzureConnectionWizardContext> | |||
|
|
|||
| public async prompt(context: T): Promise<void> { | |||
| context.dtsHub = (await context.ui.showQuickPick(await this.getPicks(context), { | |||
There was a problem hiding this comment.
UX nit: placeholder casing. The placeholder now reads "Select a durable task hub" (line 26). Other pickers often capitalize proper nouns. Consider "Select a Durable Task Hub" for consistency.
Review generated with Copilot
There was a problem hiding this comment.
I often make these lower case if not prefixed with Azure. If enough people want me to make this upper case, I can do that
MicroFish91
changed the title
Summary: